Skip to content

Sheffield | 26-ITP-jan | Richard Frimpong | Sprint 1 | Data Groups#1032

Open
Richiealx wants to merge 8 commits intoCodeYourFuture:mainfrom
Richiealx:coursework/sprint-1-data-groups
Open

Sheffield | 26-ITP-jan | Richard Frimpong | Sprint 1 | Data Groups#1032
Richiealx wants to merge 8 commits intoCodeYourFuture:mainfrom
Richiealx:coursework/sprint-1-data-groups

Conversation

@Richiealx
Copy link

@Richiealx Richiealx commented Mar 17, 2026

Learners, PR Template

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

This pull request contains my completed work for Sprint 1 - Data Groups.

Fix

Completed the fix exercise by correcting the implementation of:

  • Sprint-1/fix/median.js

Changes made:

  • fixed the median calculation
  • handled both odd and even length arrays correctly
  • filtered out non-numeric values
  • returned null when no numeric values are present
  • avoided mutating the original input array

Implement

Completed the implement exercises by implementing the required functions in:

  • Sprint-1/implement/sum.js
  • Sprint-1/implement/max.js
  • Sprint-1/implement/dedupe.js

I also completed the related test files:

  • Sprint-1/implement/sum.test.js
  • Sprint-1/implement/max.test.js
  • Sprint-1/implement/dedupe.test.js

Implemented functionality includes:

  • sum - calculates the total of numeric values in an array while ignoring non-numeric elements
  • findMax - returns the largest numeric value in an array while ignoring non-numeric elements
  • dedupe - removes duplicate values from an array while preserving the first occurrence

Refactor

Completed the refactor exercise by updating:

  • Sprint-1/refactor/includes.js

Changes made:

  • replaced the indexed for loop with a for...of loop
  • simplified the implementation while keeping the same behaviour
  • confirmed the function still correctly checks whether the target value exists in the array

Testing

All tests were run locally using:

cd Sprint-1
npm test -- fix
npm test -- implement
npm test -- refactor

@Richiealx Richiealx added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 17, 2026
cjyuan
cjyuan reviewed Mar 19, 2026
View reviewed changes
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is good. I only have a few suggestions.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 19, 2026
@Richiealx Richiealx added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2026
cjyuan
cjyuan reviewed Mar 19, 2026
View reviewed changes
Sprint-1/fix/median.js Outdated
Comment on lines +14 to +26
<<<<<<< HEAD
// Keep only real numeric values
const numbersOnly = list.filter(
(item) => typeof item === "number" && !Number.isNaN(item)
);

// Return null if the array contains no numbers
=======
// filter() returns a new array, so this does not modify the original input
const numbersOnly = list.filter((item) => Number.isFinite(item));

// Return null if there are no numeric values
>>>>>>> a22ed15 (Address mentor feedback for sprint 1 data groups)
Copy link
Contributor

@cjyuan cjyuan Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a unresolved merge conflict. You need to delete the "merge conflict markers" and the code that you do not want to keep.

There are several others on the files in this branch.

Can you fix them?

Copy link
Author

@Richiealx Richiealx Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out.

I have now resolved all merge conflicts by removing the conflict markers and keeping the correct final versions of the code.

I verified that no conflict markers remain, re-ran all Sprint 1 tests, and confirmed everything is passing before pushing the updated changes.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 19, 2026
@Richiealx Richiealx added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 20, 2026
cjyuan
cjyuan reviewed Mar 20, 2026
View reviewed changes
Sprint-1/implement/dedupe.test.js
Comment on lines +26 to +32
test("given an array with no duplicates, it returns a copy of the original array", () => {
const input = [1, 2, 3];
const result = dedupe(input);

// Given an array of strings or numbers
expect(result).toEqual(input);
expect(result).not.toBe(input);
});
Copy link
Contributor

@cjyuan cjyuan Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a chance both tests on lines 30-31 pass, but result and input have incorrect elements. Can you figure out why?

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Richiealx @cjyuan